Skip to content

feat: Git sync - add first class support for CAs#1154

Draft
adwk67 wants to merge 4 commits intomainfrom
feat/gitsync-ca-support
Draft

feat: Git sync - add first class support for CAs#1154
adwk67 wants to merge 4 commits intomainfrom
feat/gitsync-ca-support

Conversation

@adwk67
Copy link
Member

@adwk67 adwk67 commented Feb 20, 2026

Description

Part of stackabletech/issues#820

Tested with Nifi changes:

--- PASS: kuttl (214.10s)
    --- PASS: kuttl/harness (0.01s)
        --- PASS: kuttl/harness/custom-components-git-sync_nifi-2.7.2_zookeeper-latest-3.9.4_openshift-false (211.59s)
PASS

...and Airflow changes:

--- PASS: kuttl (164.37s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/ca-cert_airflow-latest-3.1.6_openshift-false (163.61s)
PASS

CRD change

Important

Since this was approved (25.02.2026) I've changed the ca-cert field to use the existing tls struct.

New optional field caCertSecretName containing a single field ca.crt.

Airflow:

    dagsGitSync:
     - repo: ...
       credentials:
         basicAuthSecretName: git-credentials
       gitFolder: "mount-dags-gitsync/dags_airflow3"
       wait: 5s
       tls:  #     <-- Optional new field
        verification:
          server:
            caCert:
              secretClass: git-ca-cert

Nifi:

       - repo: ...
         branch: ef61c87311ad2f57484c33245c9ed50908a1c785
         gitFolder: tests/templates/kuttl/custom-components-git-sync/python-processors
         tls:  #     <-- Optional new field
          verification:
            server:
              caCert:
                secretClass: git-ca-cert

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

  • Changes are OpenShift compatible
  • CRD changes approved
  • CRD documentation for all fields, following the style guide.
  • Integration tests passed (for non trivial changes)
  • Changes need to be "offline" compatible

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • (Integration-)Test cases added
  • Documentation added or updated. Follows the style guide.
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Feature Tracker has been updated
  • Proper release label has been added

@NickLarsenNZ
Copy link
Member

NickLarsenNZ commented Feb 24, 2026

caCertSecretName: git-ca-cert #     <-- Optional new field

Would there be a well defined key to lookup within the secret, and can it be overridden?

A similar PR deals with this: stackabletech/secret-operator#679, maybe useful.


Also, should we also support CA certificates in ConfigMaps?

@adwk67
Copy link
Member Author

adwk67 commented Feb 24, 2026

Would there be a well defined key to lookup within the secret, and can it be overridden?

@NickLarsenNZ The secret has single field ca.crt which is passed through to git-config for use with the http.sslCAInfo flag, so it would seem to be independent of the kubernetes distro. I've had a look here and there doesn't seem to be any information as to whether this field name is fixed or not.

If git is built against openssl then it has to be in PEM format, but the name doesn't matter: https://docs.openssl.org/3.3/man3/SSL_CTX_load_verify_locations/#notes

@sbernauer
Copy link
Member

sbernauer commented Feb 26, 2026

Thanks for raising the decision!
Is there any particular reason we can't/don't want to use the standard TLS server verification mechanism?
That way we should also be able to use framework code from operator-rs regarding volumes, mounts and stuff, see

pub struct TlsClientDetails {
/// Use a TLS connection. If not specified no TLS will be used.
pub tls: Option<Tls>,
}
and following

@adwk67
Copy link
Member Author

adwk67 commented Feb 26, 2026

Is there any particular reason we can't/don't want to use the standard TLS server verification mechanism?

Not really, other than that we only need the two cases None, or a secret name, rather than the three that Option<Tls> offers. I'll try it later today and see what the code looks like.

@sbernauer
Copy link
Member

If Git offers a flag --i-dont-care-about-certs it would be great to support that as well :) In which case we end up with 3 variants anyway

@adwk67
Copy link
Member Author

adwk67 commented Mar 3, 2026

If Git offers a flag --i-dont-care-about-certs it would be great to support that as well :) In which case we end up with 3 variants anyway

I'd prefer to only support two options - using ca-cert or not (falling back to default of broadly accepted certs) - as otherwise we silently turn off a security check. I think this should be explicit, so have added this note to the docs:

...
If this field is set to webPki: {} or is omitted altogether, then no changes will be made to the gitsync command and it will default to presenting no certificate to the backend.
Omitting this field is non-breaking behaviour and as such it does not set http.sslverify to false as disabling security checks should be a last resort and not something activated by default.
This can still be achieved by passing --git-config: http.sslverify=false explicitly.

@sbernauer What do you think?

@sbernauer
Copy link
Member

I mean the TlsClientDetails supports 3 ways of validating TLS, we use (and support if possible) that everywhere already. I don't see a reason to not support it here as well.
However, I'm totally fine defaulting to webPki, as that works out of the box for github.com and gitlab.com etc.
We have a platform issue to defaults to webPki by default, this could be a good first step.
I spiked something with claude, WDYT of this?

diff --git a/crates/stackable-operator/crds/DummyCluster.yaml b/crates/stackable-operator/crds/DummyCluster.yaml
index 1e2fb69..8cec201 100644
--- a/crates/stackable-operator/crds/DummyCluster.yaml
+++ b/crates/stackable-operator/crds/DummyCluster.yaml
@@ -143,7 +143,12 @@ spec:
                     format: uri
                     type: string
                   tls:
-                    description: Use a TLS connection. If not specified no TLS will be used.
+                    default:
+                      verification:
+                        server:
+                          caCert:
+                            webPki: {}
+                    description: Configure a TLS connection. If not specified it will default to webPki validation.
                     nullable: true
                     properties:
                       verification:
diff --git a/crates/stackable-operator/src/commons/tls_verification.rs b/crates/stackable-operator/src/commons/tls_verification.rs
index 1e399b0..d5d2b40 100644
--- a/crates/stackable-operator/src/commons/tls_verification.rs
+++ b/crates/stackable-operator/src/commons/tls_verification.rs
@@ -26,6 +26,7 @@ pub enum TlsClientDetailsError {
     },
 }
 
+#[repr(transparent)]
 #[derive(
     Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize,
 )]
@@ -35,6 +36,40 @@ pub struct TlsClientDetails {
     pub tls: Option<Tls>,
 }
 
+#[repr(transparent)]
+#[derive(
+    Clone, Debug, Deserialize, Eq, Hash, JsonSchema, Ord, PartialEq, PartialOrd, Serialize,
+)]
+#[serde(rename_all = "camelCase")]
+pub struct TlsClientDetailsWithSecureDefaults {
+    /// Configure a TLS connection. If not specified it will default to webPki validation.
+    #[serde(default = "default_web_pki_tls")]
+    pub tls: Option<Tls>,
+}
+
+impl std::ops::Deref for TlsClientDetailsWithSecureDefaults {
+    type Target = TlsClientDetails;
+
+    fn deref(&self) -> &TlsClientDetails {
+        // SAFETY: both types are `#[repr(transparent)]` over `Option<Tls>`, so they share
+        // the same memory layout and this cast is sound.
+        //
+        // This cannot silently break due to struct changes: `#[repr(transparent)]` requires
+        // exactly one non-zero-sized field, so adding a second real field to either struct
+        // is a compile error. The only scenario that would NOT be caught at compile time is
+        // deliberately removing `#[repr(transparent)]` from one of the two structs.
+        unsafe { &*(self as *const Self as *const TlsClientDetails) }
+    }
+}
+
+fn default_web_pki_tls() -> Option<Tls> {
+    Some(Tls {
+        verification: TlsVerification::Server(TlsServerVerification {
+            ca_cert: CaCert::WebPki {},
+        }),
+    })
+}
+
 impl TlsClientDetails {
     /// This functions adds
     ///
@@ -165,3 +200,50 @@ pub enum CaCert {
     /// so if you got provided with a CA cert but don't have access to the key you can still use this method.
     SecretClass(String),
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::utils::yaml_from_str_singleton_map;
+
+    #[test]
+    fn tls_client_details_with_secure_defaults_deserialization() {
+        // No tls key at all → WebPki default kicks in
+        let parsed: TlsClientDetailsWithSecureDefaults =
+            yaml_from_str_singleton_map("{}").expect("failed to deserialize empty input");
+        assert_eq!(parsed.tls, default_web_pki_tls());
+
+        // Explicit null → opt out of TLS entirely
+        let parsed: TlsClientDetailsWithSecureDefaults =
+            yaml_from_str_singleton_map("tls: null").expect("failed to deserialize tls: null");
+        assert_eq!(parsed.tls, None);
+
+        // Explicit SecretClass value is preserved as-is
+        let parsed: TlsClientDetailsWithSecureDefaults = yaml_from_str_singleton_map(
+            "tls:
+               verification:
+                 server:
+                   caCert:
+                     secretClass: my-ca",
+        )
+        .expect("failed to deserialize secretClass");
+        assert_eq!(
+            parsed.tls,
+            Some(Tls {
+                verification: TlsVerification::Server(TlsServerVerification {
+                    ca_cert: CaCert::SecretClass("my-ca".to_owned()),
+                }),
+            })
+        );
+    }
+
+    #[test]
+    fn tls_client_details_with_secure_defaults_deref() {
+        let secure: TlsClientDetailsWithSecureDefaults =
+            yaml_from_str_singleton_map("{}").expect("failed to deserialize");
+
+        // Deref must not panic and must expose the same tls value
+        let tls_client_details: &TlsClientDetails = &*secure;
+        assert_eq!(tls_client_details.tls, secure.tls);
+    }
+}
diff --git a/crates/stackable-operator/src/crd/git_sync/mod.rs b/crates/stackable-operator/src/crd/git_sync/mod.rs
index ca8e8bc..a289c6c 100644
--- a/crates/stackable-operator/src/crd/git_sync/mod.rs
+++ b/crates/stackable-operator/src/crd/git_sync/mod.rs
@@ -8,8 +8,8 @@ use stackable_shared::time::Duration;
 use url::Url;
 
 use crate::{
-    commons::tls_verification::TlsClientDetails, crd::git_sync::v1alpha2::Credentials,
-    versioned::versioned,
+    commons::tls_verification::TlsClientDetailsWithSecureDefaults,
+    crd::git_sync::v1alpha2::Credentials, versioned::versioned,
 };
 
 mod v1alpha1_impl;
@@ -77,7 +77,7 @@ pub mod versioned {
         /// If `http.sslCAInfo` is also set via `gitSyncConf` (the `--git-config` option) then a warning will be logged.
         /// If not specified no TLS will be used, defaulting to github/lab using commonly-recognised certificates.
         #[serde(flatten)]
-        pub tls: TlsClientDetails,
+        pub tls: TlsClientDetailsWithSecureDefaults,
     }
 
     #[derive(strum::Display, Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]

@adwk67
Copy link
Member Author

adwk67 commented Mar 4, 2026

Thank you! - I'll give that a try.
But don't we have an issue with this not being honoured when parsed client-side?

 // Explicit null → opt out of TLS entirely

I seem to remember that we cannot set tls: null with e.g. kafka without it defaulting to the given default value (unless we use server-side apply). Or is that the issue that will be fixed elsewhere?

@sbernauer
Copy link
Member

Or is that the issue that will be fixed elsewhere?

Nope, sadly not. We didn't get behind why sometimes you need a server side apply, sometimes it's fine without

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants